Fsw interface 2 #56
Open
Fsw interface 2 #56
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uses an install error on some systems
There was a problem hiding this comment.
Pull Request Overview
This PR integrates an updated Flight Software interface into the Monte Carlo simulation, extends the physics models (power, gravity, drag, SRP), restructures configuration parameters, and overhauls the visualization pipeline.
- Restructured
params.yamlto centralize simulation and spacecraft parameters with new flags and hardware specs - Introduced power dynamics modules and hooked them into the rigid‐body integrator
- Expanded gravity, drag, and SRP models; refactored and consolidated plotting code while removing obsolete web visualizer files
Reviewed Changes
Copilot reviewed 67 out of 75 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| montecarlo/configs/params.yaml | Restructured and expanded simulation parameters |
| argusim/world/physics/models/power.h | Added power dynamics interface |
| argusim/world/physics/models/power.cpp | Implemented power consumption and generation logic |
| argusim/world/physics/models/gravity.h | Declared sun/moon gravity and gravity‐gradient torque |
| argusim/world/physics/models/gravity.cpp | Implemented sun/moon gravity and gradient torque |
| argusim/world/physics/models/drag.h | Added drag torque interface |
| argusim/world/physics/models/drag.cpp | Implemented drag acceleration and torque |
| argusim/world/physics/models/SRP.h | Simplified shadow function and updated SRP interface |
| argusim/world/physics/models/SRP.cpp | Implemented shadow_factor and SRP acceleration |
| argusim/world/physics/models/MagneticField.cpp | Kept IGRF call; added TODO for IGRF version update |
| argusim/world/physics/models/CMakeLists.txt | Added power.cpp to the models library |
| argusim/world/physics/geometry.py | Added 3D geometry classes for occlusion and lookup table generation |
| argusim/world/physics/RigidBody.h | Updated dynamics signatures to include new models and flags |
| argusim/world/physics/RigidBody.cpp | Integrated power and actuator dynamics, updated RK4 integration |
| argusim/world/math/utils_and_transforms.h | Added SliceDef and quaternion‐conversion declarations |
| argusim/world/math/utils_and_transforms.cpp | Implemented vectorToQuaternion utility |
| argusim/world/init.py | Removed deprecated quaternion imports |
| argusim/world/LUT_generator.py | Added lookup‐table generator for geometry and sensors |
| argusim/visualization/web_visualizer/templates/home_page.html | Removed outdated web UI template |
| argusim/visualization/web_visualizer/templates/compare.html | Removed outdated web UI template |
| argusim/visualization/web_visualizer/static/css/style.css | Removed outdated web CSS |
| argusim/visualization/web_visualizer/job_comparison_tool.py | Removed obsolete comparison tool |
| argusim/visualization/sensor_plots.py | Refactored discrete sensor plotting functions |
| argusim/visualization/plotter/plots.py | Removed legacy plotter and consolidated plotting modules |
| argusim/visualization/plotter/plot_pointing.py | Removed deprecated pointing plots |
| argusim/visualization/plotter/plot_menu.py | Removed obsolete plot‐selection menu |
| argusim/visualization/plotter.py | Refactored main plotting entry‐point |
| argusim/visualization/plots.py | Introduced unified visualization module |
| argusim/visualization/plot_true_states.py | Extracted true‐state plotting helpers |
Comments suppressed due to low confidence (3)
argusim/world/math/utils_and_transforms.h:88
- [nitpick] The
vectorToQuaternionfunction lacks a docstring describing the expected vector format and behavior. Add a docstring to clarify its purpose and input requirements.
Quaternion vectorToQuaternion(const Eigen::VectorXd& vec);
argusim/world/physics/models/drag.cpp:15
- [nitpick] This TODO calls out a hard-coded Earth rotation vector. Either implement a proper source (e.g., SPICE) or clarify in documentation when the approximation is acceptable.
// [TODO]: Get more accurate earth rotation vector
montecarlo/configs/params.yaml:11
- [nitpick] There are multiple
use*flags at the top level; grouping them under a dedicated subsection (e.g., PerturbationFlags) would improve readability and organization.
useSun : False
| normal = np.cross(vector1, vector2) | ||
| return normal / np.linalg.norm(normal) | ||
|
|
||
| def is_intersecting_edge(self, point, direction, Edge): |
There was a problem hiding this comment.
[nitpick] The method returns either False or a numpy array, leading to inconsistent return types. Consider returning None for no intersection and an array for intersection to clarify its behavior.
Comment on lines
+71
to
+72
| { // VectorXd resistances, | ||
| // VectorXd power_consumption = control_input(u_idx_map["mtb_volt"].to_seq()).array() * control_input(u_idx_map["mtb_volt"].to_seq()).array() / resistances.array(); |
There was a problem hiding this comment.
[nitpick] There are outdated commented‐out code lines referring to unused variables. Remove or update these comments to keep the implementation clean and accurate.
Suggested change
| { // VectorXd resistances, | |
| // VectorXd power_consumption = control_input(u_idx_map["mtb_volt"].to_seq()).array() * control_input(u_idx_map["mtb_volt"].to_seq()).array() / resistances.array(); | |
| { |
Comment on lines
7
to
+12
| double mu = 3.98600435507e14; | ||
| double J2 = 1.08262668e-3; | ||
| double R_earth = 6.378137e6; | ||
| double mu_sun = 1.32712440018e20; | ||
| double mu_moon = 4.902801e12; |
There was a problem hiding this comment.
Global gravity constants are defined as non‐const variables. Consider marking them as static constexpr or const to avoid multiple definitions and clearly indicate they are immutable constants.
Suggested change
| double mu = 3.98600435507e14; | |
| double J2 = 1.08262668e-3; | |
| double R_earth = 6.378137e6; | |
| double mu_sun = 1.32712440018e20; | |
| double mu_moon = 4.902801e12; | |
| static constexpr double mu = 3.98600435507e14; | |
| static constexpr double J2 = 1.08262668e-3; | |
| static constexpr double R_earth = 6.378137e6; | |
| static constexpr double mu_sun = 1.32712440018e20; | |
| static constexpr double mu_moon = 4.902801e12; |
Comment on lines
+19
to
+22
| if (sd >= 0.0) { | ||
| return 1.0; | ||
| } else if (c < (b - a)) { | ||
| return 0.0; | ||
| } else { | ||
| double x = (c * c + a * a - b * b) / (2 * c); | ||
| double y = sqrt(a * a - x * x); | ||
| double A = a * a * acos(x / a) + b * b * acos((c - x) / b) - c * y; | ||
| double nu = 1 - A / (M_PI * a * a); | ||
| return nu; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The early return on sd >= 0.0 may classify shadowed points as fully illuminated. Review the shadow‐factor logic to ensure correct handling of partial or full eclipse conditions.
argusim/world/physics/RigidBody.cpp
Outdated
| VectorXd x_old = x; | ||
| VectorXd mtb_currents = x(SC.x_idx_map["mtb_currents"].to_seq()); | ||
| VectorXd mtb_volt = u(SC.u_idx_map["mtb_volt"].to_seq()); | ||
| x_old(SC.x_idx_map["mtb_currents"].to_seq()) = SC.MTB.getVoltageOrCurrent(mtb_volt, mtb_currents, "first"); |
There was a problem hiding this comment.
[nitpick] The integration stage is selected via string literals like "first"/"half"/"full", which can lead to typos. Consider using an enum or named constants for clarity and safety.
plot fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes: